fix: deprecate --*.disabled CLI flags, wire up --mcp.disabled#3482
Open
souvikghosh04 wants to merge 3 commits intomainfrom
Open
fix: deprecate --*.disabled CLI flags, wire up --mcp.disabled#3482souvikghosh04 wants to merge 3 commits intomainfrom
souvikghosh04 wants to merge 3 commits intomainfrom
Conversation
- Hide --rest.disabled, --graphql.disabled, --mcp.disabled from --help (Hidden=true) - Add [Deprecated] prefix to HelpText for all three flags - Add missing deprecation warning for --mcp.disabled - Wire --mcp.disabled through TryDetermineIfApiIsEnabled (was previously a no-op) - Remove unused TryDetermineIfMcpIsEnabled helper - Use resolved enabled booleans for REST+GraphQL disabled-together check - Add 5 MCP test rows to TestEnabledDisabledFlagsForApis
souvikghosh04
requested review from
Aniruddh25,
JerryNixon,
RubenCerna2079,
aaronburtle,
anushakolan,
sourabh1007 and
vadeveka
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves dab init CLI UX by deprecating the --*.disabled flags (while keeping them functional), wiring up the previously no-op --mcp.disabled, and expanding tests to cover MCP enabled/disabled combinations.
Changes:
- Hide
--rest.disabled,--graphql.disabled,--mcp.disabledfrom help output and mark them as deprecated via HelpText + warnings. - Wire
--mcp.disabledthrough the shared enabled/disabled resolution logic and remove the MCP-specific helper. - Extend end-to-end CLI tests to validate MCP enabled/disabled flag behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/ConfigGenerator.cs | Adds MCP deprecation warning, wires --mcp.disabled into the common resolver, and adjusts REST+GraphQL disabled validation logic. |
| src/Cli/Commands/InitOptions.cs | Hides deprecated --*.disabled flags from --help and prefixes HelpText with [Deprecated]. |
| src/Cli.Tests/EndToEndTests.cs | Adds MCP cases to enabled/disabled flag matrix and asserts MCP runtime config values. |
Comments suppressed due to low confidence (1)
src/Cli/ConfigGenerator.cs:250
- The init-time validation currently fails when both REST and GraphQL are disabled, even if MCP is enabled. This is stricter than runtime validation (RuntimeConfigValidator.ValidateGlobalEndpointRouteConfig only rejects the config when REST+GraphQL+MCP are all disabled), and it prevents generating an MCP-only config via
dab init --rest.enabled false --graphql.enabled false.
Consider updating this check to only error when all three endpoints are disabled (and update the error message accordingly).
if (!restEnabled && !graphQLEnabled)
{
_logger.LogError("Both Rest and GraphQL cannot be disabled together.");
return false;
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
disabled#3376dab inithas both--rest.disabledand--rest.enabled(same for graphql/mcp) for the same property. This is confusing UX. Additionally,--mcp.disabledwas defined but never wired up (no-op).What is this change?
--rest.disabled,--graphql.disabled,--mcp.disabledfrom--helpoutput (Hidden = true)[Deprecated]prefix to HelpText for all three flags--mcp.disabled(was missing — REST and GraphQL already had them)--mcp.disabledthroughTryDetermineIfApiIsEnabled(was previously a no-op)TryDetermineIfMcpIsEnabledhelperenabledbooleans instead of rawDisabledflagsBreaking change analysis
--*.disabledflags still work — they are just hidden from--help--mcp.disabledwas previously a no-op; now it actually disables MCPHow was this tested?
Sample Request(s)
NA